runtime: block signal_recv on targets without signal delivery (#5619) - #5620
Conversation
|
Independently verified this fix, from outside the CI harness: applied just the
The |
There was a problem hiding this comment.
🟡 Changes recommended
The new test will fail to link on Windows without an exclusion or Windows signal stubs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents signal.Notify from starving cooperative schedulers on targets without signal delivery.
Changes:
- Blocks stubbed signal reception indefinitely.
- Adds and registers a scheduler-progress regression test.
- Defines the expected test output.
File summaries
| File | Description |
|---|---|
testdata/signalnotify.txt |
Defines expected output. |
testdata/signalnotify.go |
Tests progress after signal.Notify. |
src/runtime/signalstub.go |
Parks unsupported signal reception. |
main_test.go |
Registers the test, but does not exclude unsupported Windows hosts. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "print.go", | ||
| "reflect.go", | ||
| "signal.go", | ||
| "signalnotify.go", |
|
I have not really looked into this PR, but it for sure needs a couple of things:
Thanks! |
6c61020 to
6fc5dfd
Compare
|
I've rebased, simplified the comments, and addressed the Copilot Windows skipping suggestion. |
|
Thanks @neomantra for the rebase and the updates. Almost there, just one little thing. See below, edited from an automated review:
if options.GOOS == "windows" {
switch name {
case "signal.go", "signalnotify.go":
// os/signal does not link on Windows.
continue
}
}
if isWebAssembly || isBaremetal {
switch name {
case "signal.go":
// Signals only work on POSIX-like systems.
continue
}
} |
6fc5dfd to
42470f6
Compare
The signal_recv stub returns immediately. After signal.Notify starts the signal goroutine, os/signal.loop calls the stub continuously. On cooperative schedulers, this prevents other goroutines from running. On js/wasm, it also prevents control from returning to the host. Call deadlock() to block the signal goroutine permanently. These targets cannot deliver signals. Add a regression test that calls signal.Notify and then sleeps. The test checks that the main goroutine can resume and exit. Skip this test on Windows, which has no signal implementation. Fixes tinygo-org#5619 Signed-off-by: Evan Wies <evan@neomantra.net>
42470f6 to
ff13b73
Compare
|
I've rebased this and addressed that comment, thanks! |
deadprogram
left a comment
There was a problem hiding this comment.
Thanks for this @neomantra
This was worked through with LLM. The text below is LLM-generated and I have read and reviewed all the code.
But I did finally get the BubbleTea List demo running in browser, compiled by TinyGo!
Summary
Make the stubbed
os/signal.signal_recvblock forever instead of returningimmediately, so
signal.Notifyno longer starves cooperative schedulers onwasm and baremetal targets.
Fixes #5619
Problem
src/runtime/signalstub.go(build tagstinygo.wasm || baremetal) stubbedsignal_recvasreturn ^uint32(0). Upstream'sos/signal.loopcallssignal_recvin a tight loop with no yield point — it relies on theruntime blocking until a signal arrives, as the POSIX implementation in
runtime_unix.godoes. With the immediate-return stub, the watchergoroutine started by
signal.Notifyspins forever.On a cooperative scheduler a spinning goroutine is always runnable, so the
scheduler never idles: on wasm,
_startnever returns to the host eventloop and the browser tab (or node/wazero) pins a core with all other
goroutines starved. Any program calling
signal.Notifyis affected —Bubble Tea does so unconditionally, which is how this surfaced (a V8
profile of the hung program showed 99.7% of ticks in
os/signal.loop).Fix
signal_recvnow callsdeadlock()— the same primitive a blocking emptyselectuses — parking the watcher goroutine forever. That matches thereal implementation's observable behavior on a system where no signal ever
arrives:
Notifysucceeds, the channel simply never receives anything, andeverything else keeps running. This is also what gc's js/wasm port does.
Verification
testdata/signalnotify.go(signal.Notify, thentime.Sleep, then printdone), registered for all platforms. Theexisting
signal.gotest is skipped on wasm/baremetal/windows, which iswhy this had no coverage.
dev@ 86d58db the wasm test hangs until the Go testtimeout kills it.
TestBuild/WebAssembly/signalnotify.goandTestBuild/Host/signalnotify.goboth pass (the host run exercises thereal POSIX
signal_recvpath, guarding both implementations).bubbles/listapp compiled forGOOS=js GOARCH=wasmpreviously froze the browser tab at 100% CPU rightafter startup; with this change it runs interactively.
deadlock()is defined by all four scheduler implementations(cooperative, threads, cores, none), covering the stub's whole build-tag
surface.
(this environment lacks the LLVM source checkout for compiler-rt); CI
covers those. If
os/signalturns out not to fit AVR flash, the test canbe excluded for AVR the way
json.go/stdlib.goare.Context
Found while verifying the large-parameter-spilling work (#5615) in a real
browser; it is independent of that change and reproduces on stock
dev.